Add support for GitHub Environments for Pro/Teams pricing plans #2611
Conversation
|
@gmlewis Please kindly check if this implementation makes sense. I have tested the code locally for:
private enterprise repos should have the same 'route' as the free plan public repos. Unfortunately, I don't know how to extend the testing function to test the different 'paths' |
Codecov Report
@@ Coverage Diff @@
## master #2611 +/- ##
=======================================
Coverage 97.99% 98.00%
=======================================
Files 127 127
Lines 10948 10969 +21
=======================================
+ Hits 10729 10750 +21
Misses 150 150
Partials 69 69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Thank you, @AnitaErnszt !
We definitely need to add in tests for the special cases, and it should be relatively simple.
In a new test, instead of returning a 200 OK, your could would initially return a 422, and then your test would look like all the others and return new responses.
Alternatively, and maybe this would be the best approach anyway...
Instead of making a huge indented block like we have now, you could make a new method that is NOT exported... something like createNewEnvNoEnterprise and then simply call it:
if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity && environment != nil && len(environment.Reviewers) == 0 && environment.GetWaitTimer() == 0 {
return s.createNewEnvNoEnterprise(u, environment)
}Then that way, you can have two new test functions... one just to make sure the 422 triggers the if and the other test function can fully test out createNewEnvNoEnterprise (like we already do for every other existing endpoint method). Does that make sense?
|
@gmlewis I have added the tests. |
|
On another note the contributor guide is recommending to run |
There was a problem hiding this comment.
Thank you, @AnitaErnszt !
LGTM.
Awaiting second LGTM+Approval before from any other contributor to this repo before merging.
|
Thank you, @xorima ! |
The PR fixes the issue reported on #2602
In June GitHub made the environments and environment protection rules generally available, however, the wait_timer and reviewers remain limited to enterprise.
Code was written to send
wait_timerandreviewerswith default values if no value is present, so existing settings can be cleared.Due to this, the code was returning 422 when called to create environments for Pro/Team private repos, as these parameters are only supported for enterprise or public repos.
PR intends to handle the error with a retry when the following conditions are met:
When retrying the call, a new object is sent, which only contains the parameters that are accepted for Pro/Teams private repos.